Repository navigation
[LinearSolver.Direct] BTDLinearSolver: do not clear the dense inverse at each factorization - #6433
Merged
Merged
Conversation
… at each factorization invert() resized Minv at every factorization, and BlockFullMatrix::resize() always clears all its nb*nb blocks. Clearing is not needed: a block of Minv is only read after computeMinvBlock() has written it (tracked by nBlockComputedMinv, reset at each factorization). Minv is now only resized when its size changes. The previous behaviour can be restored with the new forceClearInverse Data.
Clearing the dense inverse is never needed: every block of Minv is written (plain assignment) by computeMinvBlock() before being read, and nBlockComputedMinv is reset at each factorization, whatever the topology. A size change still goes through resize(), as before. Checked by filling Minv with NaN instead of clearing it: no NaN read and bit-identical trajectories on the BTD example scenes, including subpartSolve and partial_solve (unbuilt Gauss-Seidel) paths.
fredroy
force-pushed
the
fix_btd_minv_clear
branch
from
October 30, 2026 01:15
a92b1c6 to
e0aa6d5
Compare
EulalieCoevoet
approved these changes
Nov 2, 2026
EulalieCoevoet
left a comment
Contributor
There was a problem hiding this comment.
Sounds and looks good to me. The method resize already checks if the resize is needed. However it also always clears the matrix, which can be expensive.
If I understood the code correctly, I agree with the comment that this clear is not necessary.
bakpaul
approved these changes
Nov 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
invert() resized Minv at every factorization, and BlockFullMatrix::resize() always clears all its nb*nb blocks.
It appeared that clear() seems superfluous, as the block as always written before being read.
Empirical checks:
and Claude tested by filling the Minv with NaN instead of clearing -> 0 NaN reads.
Not really a real proof but seems OK for me © 😅
Results: use case in BeamAdapter,
Iin 3instruments_collis, the default tools has 180 nodes, results are roughly similar
if we increase the number of nodes (only in the mechanical object) to e.g 580 nodes
and 1080 nodes
my first version was introducing a Data to force clearing anyway (by default) to keep the compatibility, but removed it anyway as it seems totally useless.
Note that it will clear anyway if the size is different between factorization
[with-all-tests]
By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).
Reviewers will merge this pull-request only if